fix: guard handle_sdk_message against non-map JSON#51
Merged
Conversation
The CLI may emit non-map JSON values (booleans, arrays, numbers, strings)
on stdout when, for example, a hook callback fails and a Zod schema
validation error is printed alongside the normal stream-JSON output.
Control.classify/1 falls through to {:message, json} for any non-map, so
without a guard the subsequent handle_sdk_message/2 clauses call
json["type"], which invokes Access.get/3 on a non-map and crashes the
Port GenServer with a FunctionClauseError — aborting the in-flight
session.
Add a guard clause that logs and drops the offending chunk so the
session can continue. Covered by a MockCLI integration test that streams
true, [1,2,3], and 42 between a system message and the final result and
asserts the session still completes with the expected result.
Owner
|
@pshoukry thanks for the PR! Looks like dialyzer is failing. I'm thinking we could maybe do this check one level higher at Something like this: {:ok, json} when is_map(json) ->
case Control.classify(json) do ...
{:ok, non_map} ->
Logger.warning("Dropping non-map CLI output: #{inspect(non_map)}")
state
{:error, _} ->
Logger.debug("Non-JSON CLI output: ...")
state
What do you think? |
Move the guard one level up so non-map CLI output (booleans, arrays, numbers, strings) never reaches Control.classify/1 — which is spec'd to take a map and was failing dialyzer when passed a non-map. Also avoids misclassifying non-map values as SDK messages via the classify/1 catch-all.
Contributor
Author
|
@guess sounds good, please check. |
Contributor
Author
|
@guess I applied the fix as you suggested. please check. |
Owner
|
@pshoukry looks like a test is failing |
Contributor
Author
The error looked to me like a network error and I wasn't sure if that passed before. I will take a deeper look. |
…lake `capture_log/2` installs a global Logger handler, so logs from concurrently running async tests (e.g. session_test.exs sending an invalid JSON `adapter_message` to provoke a warning) were leaking into these `assert log == ""` checks and failing CI nondeterministically. Refute only the specific "no matching system env" messages this code path can emit, so unrelated log noise from neighbouring tests no longer breaks the assertion.
Contributor
Author
|
@guess I pushed a fix vs the flaky test that fails locally and pushed again. I think rerunning the test suite would have passed since this error seems to be intermittent. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The Claude CLI can emit non-map JSON values (
true,[1,2,3],42, …) on stdout alongside the normal stream-JSON output — for example, when a hook callback fails and a Zod schema validation error is logged.ClaudeCode.CLI.Control.classify/1falls through to{:message, json}for any non-map, so without a guard the subsequentClaudeCode.Adapter.Port.handle_sdk_message/2clauses calljson["type"]— which invokesAccess.get/3on the non-map and crashes the PortGenServerwith aFunctionClauseError, aborting the in-flight session.Fix
Add a
when not is_map(json)clause tohandle_sdk_message/2that logs and drops the offending chunk so the session continues:Test
New integration test in
port_integration_test.exsthat uses the existingMockCLIhelper to streamtrue,[1,2,3], and42between a system message and the final result, then asserts the session completes and returns the expected%ResultMessage{}. Full suite (1492 tests) passes.Repro context
This was hit in a downstream SDK consumer when a hook returned an unexpected shape and the CLI printed Zod validation diagnostics on stdout. Without the guard the entire session terminates the moment that line is processed — even though the actual model output is fine and the result message arrives right after.